[EVAL] feat: add certificate trust configuration for MCP servers #14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add certificateTrust configuration options for SSE and StreamableHTTP transports - Support allowSelfSigned, caCertPath, and rejectUnauthorized options - Implement HTTPS agent configuration for Node.js fetch operations - Add comprehensive tests for certificate trust configuration - Add documentation explaining usage and security considerations Fixes #8355
- Use spread operator with type assertion for Node.js-specific agent property - Ensures compatibility with standard RequestInit type while allowing HTTPS agent
![roo-code-dan[bot]](https://avatars.githubusercontent.com/u/211522643?s=60&v=4){kind=small}
|
|
||
| // Handle certificate rejection | ||
| if (rejectUnauthorized === false || allowSelfSigned === true) { | ||
| agentOptions.rejectUnauthorized = false |
There was a problem hiding this comment.
The logic for handling allowSelfSigned and rejectUnauthorized has a potential conflict. When allowSelfSigned is true, it unconditionally sets rejectUnauthorized to false (line 769), even if the user explicitly configured rejectUnauthorized: true. This means allowSelfSigned: true will always override an explicit rejectUnauthorized: true setting.
Consider using a more explicit precedence or only setting rejectUnauthorized when it's not already defined:
if (rejectUnauthorized === false || (allowSelfSigned === true && rejectUnauthorized === undefined)) {
agentOptions.rejectUnauthorized = false
}This same issue exists in the SSE implementation at line 839.
| - Certificate trust settings only apply to SSE and StreamableHTTP transports | ||
| - STDIO transport servers do not use HTTPS and therefore don't need certificate configuration | ||
| - The configuration requires Node.js environment; browser-based implementations may have different requirements | ||
| - Test webhook commit 17 |
There was a problem hiding this comment.
This line contains a test artifact ("Test webhook commit 17") that should be removed before merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🧪 Evaluation PR for Testing
This is an automated test PR created for evaluation purposes.
Summary
This PR addresses Issue #8355 by adding certificate trust configuration support for MCP servers using HTTPS connections (SSE and StreamableHTTP transports).
Problem
MCP servers using self-signed or internal CA-signed certificates cannot be used because there's no way to specify trust information for these certificates. This prevents users from connecting to MCP servers in development environments or corporate networks with internal certificate authorities.
Solution
Added a certificateTrust configuration option for SSE and StreamableHTTP server types with three settings:
Changes
Testing
Security Considerations
Documentation
Added comprehensive documentation in docs/mcp-certificate-trust.md including: